Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Jetpack Compose navigation + New Homescreen UI + Amoled Theme #160

Merged
merged 7 commits into from
Jul 16, 2023

Conversation

SuhasDissa
Copy link
Member

Hi there,

I noticed that you're using Dialogs for the settings screen and recording screen. I understand that you may have done this for convenience, or maybe you didn't expect the app to have that many settings.

So, anyway I thought it might be a better idea to use Jetpack Compose navigation to navigate between different screens, mainly the settings screen and the recordings screen.

On the home screen, I didn't like the idea of using a tiny arrow to switch between the recording screens. I think it would be better to use a Bottom Bar, which would be more self-explanatory for users.

After all that, everything looked pretty good from a user experience standpoint.
However, after moving all the icons to the top app bar, the homepage started to look a bit empty and boring. I thought to myself, "I'm gonna make this more aesthetically pleasing".
So I moved some things around and added some placeholder art. I think it makes the app look less boring, and it's a technique that I've seen Google use in their apps.

I hope you will like these changes.

By the way I made these changes as smaller commits so it will be easier for you to revert any unwanted changes.

Thank you

# Conflicts:
#	app/src/main/java/com/bnyro/recorder/ui/MainActivity.kt
#	app/src/main/java/com/bnyro/recorder/ui/screens/PlayerScreen.kt
#	app/src/main/java/com/bnyro/recorder/ui/screens/RecorderScreen.kt
@SuhasDissa
Copy link
Member Author

SuhasDissa commented Jul 14, 2023

Here are some screenshots for reference


@Bnyro
Copy link
Member

Bnyro commented Jul 16, 2023

Really cool changes, thanks for your work!

I'm not sure what kind of formatter/linter you use, however I'd like the code to be formatted with ktlint so that the code looks consistent everywhere.

Two thing I noticed:

  1. Instead of using a shared preferences listener, please let us continue using view models. Therefore they need to be created in the MainActivity and passed down as arguments to components that are lower in hierarchy.
  2. When using the AmoLED / black theme, it doesn't follow the Material You / wallpaper accent colors on Android 12+, so it looks a bit weird at that places in my opinion. Not sure if there's a better way to achieve an amoled theme, but it'd be nice if it followed the Android accent color if available.

@SuhasDissa
Copy link
Member Author

I'm using the built in formatting options in android studio. (Ctrl + Alt + L)

To be honest I removed the themeModel becuse it didn't work. I wasn't sure why.

Maybe there's a way to override some specific colors in the material you dark theme. I will do some research on that later.

@Bnyro
Copy link
Member

Bnyro commented Jul 16, 2023

It doesn't work because Navigation Compose doesn't handle view models properly.
See https://stackoverflow.com/questions/68548488/sharing-viewmodel-within-jetpack-compose-navigation.

@SuhasDissa
Copy link
Member Author

SuhasDissa commented Jul 16, 2023

It doesn't work because Navigation Compose doesn't handle view models properly. See https://stackoverflow.com/questions/68548488/sharing-viewmodel-within-jetpack-compose-navigation.

I use navigation in all my apps. but I never had this problem. Maybe this is a bug in the specific version I used.
I use the latest beta version in my apps because it support navigation animation.
I had to use an older version of navigation here because your android gradle plugin version is a bit older.

For example in my MellowMusic App I use PlayerViewModel in MainActivity.kt , FullScreenPlayer.kt and MiniPlayer

@Bnyro
Copy link
Member

Bnyro commented Jul 16, 2023

I use navigation in all my apps. but I never had this problem. Maybe this is a bug in the specific version I used. I use the latest beta version in my apps because it support navigation animation. I had to use an older version of navigation here because your android gradle plugin version is a bit older.

That's strange, however in all apps I used navigation compose so far, this issue persists, no matter the version of the used dependencies I tested.
Doesn't matter anyways since the work around is quite straight forward, so it'd be good to just pass the view models down in hierarchy for now.

@SuhasDissa
Copy link
Member Author

Maybe that happens because you use viewModel as a val

I asked chatGPT about this and this was the response

Using a ViewModel as a val (variable) in Jetpack Compose is not inherently bad. In fact, it is a common practice to store a ViewModel as a val when you want to retain and access its state across recompositions within the same scope.

By storing a ViewModel as a val, you ensure that the same instance of the ViewModel is used throughout the lifetime of the surrounding component or container. This can be beneficial in scenarios where you need to preserve the ViewModel's state, handle configuration changes, or share the ViewModel with multiple composables within the same scope.

Example:

@Composable
fun MyScreen() {
    val viewModel = viewModel<MyViewModel>()

    // Use the viewModel in multiple composables within the same scope
}

However, it's essential to be mindful of the lifecycle management of the ViewModel and prevent potential issues such as memory leaks or unintended state retention. You should consider scoping the ViewModel appropriately based on the lifecycle of the surrounding component or container.

You can use the viewModel or rememberViewModel functions provided by the androidx.lifecycle.viewmodel.compose package to handle ViewModel instantiation and scoping within Jetpack Compose.

It's worth noting that Jetpack Compose encourages a more declarative and unidirectional data flow approach. Therefore, when using a ViewModel as a val, it's recommended to expose its state through State or StateFlow objects, allowing Compose to automatically handle recompositions and updates based on changes in the ViewModel's state.

It says that using val for viewModel makes it keep the state same within the Composables of the same scope.

So I bet the same thing applies for this https://stackoverflow.com/questions/68548488/sharing-viewmodel-within-jetpack-compose-navigation

@Bnyro
Copy link
Member

Bnyro commented Jul 16, 2023

That seems like it could be the reason.
However, I fix we should just stick with one of the solutions described in the Stackoverflow issue, and we're good.

@SuhasDissa
Copy link
Member Author

2. When using the AmoLED / black theme, it doesn't follow the Material You / wallpaper accent colors on Android 12+, so it looks a bit weird at that places in my opinion. Not sure if there's a better way to achieve an amoled theme, but it'd be nice if it followed the Android accent color if available.

FIxed it here.
(Ignore the mess made by the ktlint)

@Bnyro
Copy link
Member

Bnyro commented Jul 16, 2023

Oh, did you use ktlint --code-style=android -F or just ktlint? Looks somewhat not what I expected ktlint to make it look like :/

@Bnyro Bnyro merged commit 4a2e7c6 into you-apps:main Jul 16, 2023
1 check passed
@Bnyro
Copy link
Member

Bnyro commented Jul 16, 2023

I'll fix the linting with an other commit myself, thanks for your efforts!

@SuhasDissa SuhasDissa changed the title Implemented Jetpack Compose navigation + Revamp home screen Jetpack Compose navigation + New Homescreen UI + Amoled Theme Aug 5, 2023
@SuhasDissa SuhasDissa mentioned this pull request Aug 5, 2023
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants